-
Notifications
You must be signed in to change notification settings - Fork 325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add type annotations to xml.py #1327
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duck typing in this PR is incomplete. Please fix accordingly.
Sorry, which class/function are you referring to? your review comment didn't attach to any line of files. |
Code in xml.py |
done |
@@ -80,20 +92,43 @@ def findtext(element, name, strict=False): | |||
return element.text or "" | |||
|
|||
|
|||
def unmarshal(cls, xmlstring): | |||
K = TypeVar("K") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have meaningful name here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is generic type parameter, it's custom to be named as T
K
J
。it's like i
x
,y
in looping.
data, encoding=None, xml_declaration=False, | ||
) | ||
return data.getvalue() | ||
with io.BytesIO() as data: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to use with
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
close file and discard the buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary, gc will handle them if we don't use with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. For this function, it is unnecessary.
data, encoding=None, xml_declaration=False, | ||
) | ||
return data.getvalue() | ||
with io.BytesIO() as data: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. For this function, it is unnecessary.
No description provided.